Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(executor): Force no multicall on Polygon #1690

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

fix(executor): Force no multicall on Polygon #1690

wants to merge 6 commits into from
+4 −1

Conversation

pxrl
Copy link
Contributor

@pxrl pxrl commented Jul 18, 2024

Relayer refund leaves must not be batched via multicall on Polygon. The existing executor implementation will attempt multicall in cases where there are multiple relayer refund leaves, and this imposes a soft deadlock on the executor. This can be recovered by executing one leaf at a time. A better solution is just to override the defaults for Polygon in the dataworker.

@pxrl
fix(executor): Force no multicall on Polygon
066f61a 
Relayer refund leaves must not be batched via multicall on Polygon. The
existing executor implementation will attempt multicall in cases where
there are multiple relayer refund leaves, and this imposes a soft
deadlock on the executor. This can be recovered by executing one leaf at
a time. A better solution is just to override the defaults for Polygon
in the dataworker.
james-a-morris
james-a-morris approved these changes Jul 18, 2024
View reviewed changes
Copy link
Contributor

@james-a-morris james-a-morris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense - but could we maybe put this logic in constructClients and expose this override in config?

@james-a-morris
Copy link
Contributor

james-a-morris commented Jul 18, 2024 •
edited by pxrl
Loading

After some thought - we're assuming the only info coming from the config is the multiCallChunkSize which may not always be the case. Would it make sense to just override the config before we call constructCommonClients to be:

{
...config,
{ ...config.multiCallChunkSize, [CHAIN_IDs.POLYGON]: 1 }
}

@pxrl
Copy link
Contributor Author

pxrl commented Jul 18, 2024 •
edited
Loading

One clarification here - the requirement for multicall in the Polygon SpokePool is more nuanced than I originally thought. It only reverts if there is a bundled transaction that's not an executeRelayerRefundLeaf() call, so multicall in itself is not inherently problematic on Polygon.

I'm not aware of how we could be multicalling executeRelayerRefundLeaf() alongside something else, so the issue today may simply have been stuck transaction weirdness (which has been seen before on Polygon). In general though, I don't see any major problem with skipping multicall on Polygon since the transactions themselves are so cheap anyway.

    /**                                                                                                                                                                                        
     * @notice Override multicall so that it cannot include executeRelayerRefundLeaf                                                                                                           
     * as one of the calls combined with other public function calls.                                                                                                                          
     * @dev Multicalling a single transaction will always succeed.                                                                                                                             
     * @dev Multicalling execute functions without combining other public function calls will succeed.                                                                                         
     * @dev Multicalling public function calls without combining execute functions will succeed.                                                                                               
     */                                                                                                                                                                                        
    function _validateMulticallData(bytes[] calldata data) internal pure override {                                                                                                            
        bool hasOtherPublicFunctionCall = false;                                                                                                                                               
        bool hasExecutedLeafCall = false;                                                                                                                                                      
        for (uint256 i = 0; i < data.length; i++) {                                                                                                                                            
            bytes4 selector = bytes4(data[i][:4]);                                                                                                                                             
            if (selector == SpokePoolInterface.executeRelayerRefundLeaf.selector) {                                                                                                            
                if (hasOtherPublicFunctionCall) revert MulticallExecuteLeaf();                                                                                                                 
                hasExecutedLeafCall = true;                                                                                                                                                    
            } else {                                                                                                                                                                           
                if (hasExecutedLeafCall) revert MulticallExecuteLeaf();                                                                                                                        
                hasOtherPublicFunctionCall = true;                                                                                                                                             
            }                                                                                                                                                                                  
        }                                                                                                                                                                                      
    }

@pxrl
Copy link
Contributor Author

pxrl commented Jul 18, 2024

After some thought - we're assuming the only info coming from the config is the multiCallChunkSize which may not always be the case. Would it make sense to just override the config before we call constructCommonClients to be:

{ ...config, { ...config.multiCallChunkSize, [CHAIN_IDs.POLYGON]: 1 } }

That's a cleaner suggestion; yeah. This can probably be handled in the dataworker config parser.

@pxrl
Copy link
Contributor Author

pxrl commented Sep 16, 2024

I'll merge this in because I've seen stuck transactions again multiple times over the past couple of weeks.

@pxrl
Copy link
Contributor Author

pxrl commented Sep 16, 2024

@james-a-morris Just circling back to this one - I applied the change you suggested 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@james-a-morris james-a-morris james-a-morris approved these changes

@mrice32 mrice32 Awaiting requested review from mrice32 mrice32 is a code owner

@nicholaspai nicholaspai Awaiting requested review from nicholaspai nicholaspai is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pxrl @james-a-morris